Make nft web app nicer #602
Conversation
WalkthroughThe pull request introduces several modifications across three files, primarily focusing on the handling of messages and the structure of function calls in Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
prediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py (1)
146-149: Re-evaluate the necessity of filtering "system" rolesSince the "system" role has been removed from handling in
parse_function_and_body, consider whether filtering out "system" roles in this context remains necessary. Removing redundant code can improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
prediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py(6 hunks)prediction_market_agent/db/long_term_memory_table_handler.py(1 hunks)prediction_market_agent/db/sql_handler.py(1 hunks)
🔇 Additional comments (4)
prediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py (2)
14-14: Import of check_not_none looks appropriate
The addition of check_not_none import aligns with its usage in the code.
31-33: Importing LongTermMemories and LongTermMemoryTableHandler
The imported classes are used later in the code, ensuring that dependencies are correctly managed.
prediction_market_agent/db/sql_handler.py (1)
38-38: Update type hint to list[SQLModelType] for consistency
Changing the return type from t.Sequence[SQLModelType] to list[SQLModelType] clarifies the expected return type and aligns with Python's type hinting conventions.
prediction_market_agent/db/long_term_memory_table_handler.py (1)
52-52: Update return type to list[LongTermMemories] for clarity
Modifying the return type of search to return a list enhances type consistency and clarity across the codebase.
| function_call: LongTermMemories, | ||
| function_output: LongTermMemories, | ||
| ) -> None: | ||
| parsed_function, parsed_body = parse_function_and_body(role, message) | ||
| if parsed_function is None: | ||
| return | ||
| # If the message is output from one of these functions, skip it, because it's not interesting to read `The reasoning has been recorded` and similar over and over again. | ||
| if parsed_body in ( | ||
| Reasoning()(""), | ||
| BroadcastPublicMessageToHumans.OUTPUT_TEXT, | ||
| SendPaidMessageToAnotherAgent.OUTPUT_TEXT, | ||
| ): | ||
| return | ||
| created_at = function_output.datetime_ | ||
|
|
||
| parsed_function_call_name, parsed_function_call_body = parse_function_and_body( | ||
| check_not_none(function_call.metadata_dict)["role"], | ||
| check_not_none(function_call.metadata_dict)["content"], | ||
| ) | ||
| parsed_function_output_name, parsed_function_output_body = parse_function_and_body( | ||
| check_not_none(function_output.metadata_dict)["role"], | ||
| check_not_none(function_output.metadata_dict)["content"], | ||
| ) |
There was a problem hiding this comment.
Handle potential KeyError when accessing metadata_dict keys
Accessing metadata_dict["role"] and metadata_dict["content"] without checking if the keys exist may lead to a KeyError. Consider using the .get() method with default values or adding error handling to ensure robustness.
Apply this diff to prevent potential KeyError exceptions:
parsed_function_call_name, parsed_function_call_body = parse_function_and_body(
- check_not_none(function_call.metadata_dict)["role"],
- check_not_none(function_call.metadata_dict)["content"],
+ check_not_none(function_call.metadata_dict.get("role", "")),
+ check_not_none(function_call.metadata_dict.get("content", "")),
)
parsed_function_output_name, parsed_function_output_body = parse_function_and_body(
- check_not_none(function_output.metadata_dict)["role"],
- check_not_none(function_output.metadata_dict)["content"],
+ check_not_none(function_output.metadata_dict.get("role", "")),
+ check_not_none(function_output.metadata_dict.get("content", "")),
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function_call: LongTermMemories, | |
| function_output: LongTermMemories, | |
| ) -> None: | |
| parsed_function, parsed_body = parse_function_and_body(role, message) | |
| if parsed_function is None: | |
| return | |
| # If the message is output from one of these functions, skip it, because it's not interesting to read `The reasoning has been recorded` and similar over and over again. | |
| if parsed_body in ( | |
| Reasoning()(""), | |
| BroadcastPublicMessageToHumans.OUTPUT_TEXT, | |
| SendPaidMessageToAnotherAgent.OUTPUT_TEXT, | |
| ): | |
| return | |
| created_at = function_output.datetime_ | |
| parsed_function_call_name, parsed_function_call_body = parse_function_and_body( | |
| check_not_none(function_call.metadata_dict)["role"], | |
| check_not_none(function_call.metadata_dict)["content"], | |
| ) | |
| parsed_function_output_name, parsed_function_output_body = parse_function_and_body( | |
| check_not_none(function_output.metadata_dict)["role"], | |
| check_not_none(function_output.metadata_dict)["content"], | |
| ) | |
| function_call: LongTermMemories, | |
| function_output: LongTermMemories, | |
| ) -> None: | |
| created_at = function_output.datetime_ | |
| parsed_function_call_name, parsed_function_call_body = parse_function_and_body( | |
| check_not_none(function_call.metadata_dict.get("role", "")), | |
| check_not_none(function_call.metadata_dict.get("content", "")), | |
| ) | |
| parsed_function_output_name, parsed_function_output_body = parse_function_and_body( | |
| check_not_none(function_output.metadata_dict.get("role", "")), | |
| check_not_none(function_output.metadata_dict.get("content", "")), | |
| ) |
| if parsed_function_call_name == Reasoning.__name__: | ||
| # Don't show reasoning as function call, to make it a bit nicer. | ||
| st.markdown( | ||
| parsed_function_call_body.replace("reasoning='", "").replace("')", "") | ||
| ) | ||
| else: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve parsing of parsed_function_call_body for robustness
Using string replacements to parse parsed_function_call_body may not handle all cases correctly. Consider using regular expressions or parsing the arguments properly to make the code more robust.
Apply this diff to use a regular expression for parsing:
+import re
...
if parsed_function_call_name == Reasoning.__name__:
# Don't show reasoning as function call, to make it a bit nicer.
- st.markdown(
- parsed_function_call_body.replace("reasoning='", "").replace("')", "")
- )
+ reasoning_match = re.match(r"reasoning=['"](.*)['"]", parsed_function_call_body)
+ if reasoning_match:
+ reasoning_text = reasoning_match.group(1)
+ st.markdown(reasoning_text)
+ else:
+ st.markdown(parsed_function_call_body)Committable suggestion skipped: line range outside the PR's diff.
| role: t.Literal["user", "assistant"], message: str | ||
| ) -> t.Tuple[str, str]: |
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered evidence, I can now provide a final response:
The "system" role removal requires attention in specific files
The codebase still contains several references to the "system" role that need to be addressed:
prediction_market_agent/agents/microchain_agent/agent_functions.py: Uses "system" role in history assignmentprediction_market_agent/agents/microchain_agent/microchain_agent.py: Contains two instances of "system" role usageprediction_market_agent/agents/microchain_agent/memory.py: Has a comparison with "system" roleprediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py: Contains filtering logic for "system" role
While the parse_function_and_body function has been updated to exclude the "system" role, there are still active uses of it in the codebase that need to be reviewed and potentially updated for consistency.
🔗 Analysis chain
Confirm removal of the "system" role throughout the codebase
The parse_function_and_body function now excludes the "system" role. Please verify that all references to the "system" role have been updated elsewhere to prevent potential inconsistencies or errors.
Run the following script to check for any remaining references to the "system" role:
Let me gather more context about the usage of roles in the codebase, particularly around the parse_function_and_body function.
Let me try a different approach to understand the context better.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for references to the "system" role in the codebase.
# Test: Find occurrences where "role" is assigned or compared to "system"
rg 'role.*["'\'']system["'\'']'
Length of output: 1616
Script:
#!/bin/bash
# Check the implementation of parse_function_and_body and surrounding context
ast-grep --pattern 'def parse_function_and_body($$$) {
$$$
}'
# Also check for any type definitions or constants related to roles
rg -A 3 'Literal\[.*"system".*\]|ROLE_.*system'
Length of output: 115
Script:
#!/bin/bash
# Get the full content of the file containing parse_function_and_body
fd "app_nft_treasury_game.py" --exec cat {}
# Look for any role-related type definitions or enums
rg "Literal\[.*role.*\]|Role|ROLE"
Length of output: 10233
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
prediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py (1)
153-154: Reevaluate the necessity of filtering out'system'rolesSince
'system'roles are being removed from message handling, verify if filtering out'system'roles is still necessary. If'system'roles are no longer present in themetadata_dict, you may remove this filter to simplify the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
prediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py(6 hunks)
🔇 Additional comments (4)
prediction_market_agent/agents/microchain_agent/nft_treasury_game/app_nft_treasury_game.py (4)
93-99: Handle potential KeyError when accessing metadata_dict keys
Accessing metadata_dict["role"] and metadata_dict["content"] without checking for key existence may lead to a KeyError. Consider using the .get() method with default values or adding error handling to ensure robustness.
Apply this diff to prevent potential KeyError exceptions:
parsed_function_call_name, parsed_function_call_body = parse_function_and_body(
- check_not_none(function_call.metadata_dict)["role"],
- check_not_none(function_call.metadata_dict)["content"],
+ check_not_none(function_call.metadata_dict.get("role", "")),
+ check_not_none(function_call.metadata_dict.get("content", "")),
)
parsed_function_output_name, parsed_function_output_body = parse_function_and_body(
- check_not_none(function_output.metadata_dict)["role"],
- check_not_none(function_output.metadata_dict)["content"],
+ check_not_none(function_output.metadata_dict.get("role", "")),
+ check_not_none(function_output.metadata_dict.get("content", "")),
)116-120: Improve parsing of parsed_function_call_body for robustness
Using string replacements to parse parsed_function_call_body may not handle all cases correctly. Consider using regular expressions or proper argument parsing to enhance robustness.
Apply this diff to use a regular expression for parsing:
+import re
...
if parsed_function_call_name == Reasoning.__name__:
# Don't show reasoning as function call to make it nicer.
- st.markdown(
- parsed_function_call_body.replace("reasoning='", "").replace("')", "")
- )
+ reasoning_match = re.match(r"reasoning=['\"](.*)['\"]", parsed_function_call_body)
+ if reasoning_match:
+ reasoning_text = reasoning_match.group(1)
+ st.markdown(reasoning_text)
+ else:
+ st.markdown(parsed_function_call_body)153-154:
Handle potential KeyError when filtering calls based on metadata_dict["role"]
When filtering calls, accessing metadata_dict["role"] without checking for key existence may lead to a KeyError. Consider using the .get() method with a default value or adding error handling.
Apply this diff to prevent potential KeyError exceptions:
calls = [
- call for call in calls if check_not_none(call.metadata_dict)["role"] != "system"
+ call for call in calls if check_not_none(call.metadata_dict.get("role", "")) != "system"
]Likely invalid or redundant comment.
68-69: Ensure all calls to parse_function_and_body use valid role values
The parse_function_and_body function now accepts role as Literal["user", "assistant"]. Please verify that there are no calls passing 'system' as the role argument. Any such calls will raise a ValueError.
Run the following script to identify any calls with 'system' as role:
✅ Verification successful
No issues with parse_function_and_body role usage
The codebase explicitly filters out messages with role="system" before they reach parse_function_and_body. This is evident from:
- The filtering in
microchain_agent.pythat excludes system messages from history - The explicit filtering in
app_nft_treasury_game.pythat removes system messages from calls
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find calls to parse_function_and_body with 'system' as the role argument.
# Search for function calls to parse_function_and_body where role is 'system'
rg -A 2 'parse_function_and_body\(' -g "*.py" | grep -E 'role\s*=\s*["'\'']system["'\'']'
Length of output: 91
Script:
#!/bin/bash
# Let's first find all calls to parse_function_and_body to analyze their role arguments
rg -A 3 'parse_function_and_body\(' -g "*.py"
Length of output: 1718
Script:
#!/bin/bash
# Let's examine the metadata_dict to see what roles are being passed
rg -B 3 -A 1 'metadata_dict.*role' -g "*.py"
Length of output: 2627
A few changes: